Vireo ResizeArray implementation #464
Conversation
| @@ -279,20 +279,29 @@ VIREO_EXPORT Int32 EggShell_GetArrayDimLength(TypeManagerRef tm, const char* viN | |||
| } | |||
| //------------------------------------------------------------ | |||
| //! Resizes a variable size Array symbol to have new dimension lengths specified by newLengths, it also initializes cells for non-flat data. | |||
| //! Returns -1 if the symbols is not found, -2 if was not possible to resize the array and 0 if resizing was successful. | |||
| VIREO_EXPORT Int32 EggShell_ResizeArray(TypeManagerRef tm, const char* viName, const char* eltName, Int32 rank, Int32* newLengths) | |||
| VIREO_EXPORT EggShellResult EggShell_ResizeArray(TypeManagerRef tm, const TypeRef actualType, const void* pData, | |||
There was a problem hiding this comment.
Consider const also for TypeManagerRef. Also, add at least null checks for tm. We might need to update other API calls to validate tm.
There was a problem hiding this comment.
None of the other functions in CEntryPoints.cpp use const for the TypeManagerRef. Maybe we can submit that as a separate change before merging ni:api-type-refactor to master.
There was a problem hiding this comment.
Made an issue to track things we should verify across all the implemented functions before merging. Tracked as "Consistent marking of parameters as const on the CEntryPoints" in #466
| var newDimensionsLength = newDimensions.length; | ||
| var dimensionsPointer = Module.stackAlloc(newDimensionsLength * LENGTH_SIZE); | ||
| var i, currentDimension; | ||
| for (i = 0; i < newDimensionsLength; i += 1) { |
There was a problem hiding this comment.
Is it common practice is JS to use i += 1, instead of i++?
There was a problem hiding this comment.
Is a linting rule in .eslintrc.js. Why is preferred? Because ++ are subject to auto-semicolon insertion. Checkout eslint docs. If we really don't like it, there's also the exception "allowForLoopAfterthoughts": true
| ' (typeRef: ' + valueRef.typeRef + ')' + | ||
| ' (dataRef: ' + valueRef.dataRef + ')'); | ||
| } | ||
| Module.stackRestore(stack); |
There was a problem hiding this comment.
Do you know if any of the Errors thrown above would cause a memory leak?
There was a problem hiding this comment.
Written under the assumption that on exception throw to JS the whole Vireo instance (stack + heap) should be considered corrupt.
We should verify that assumption is usable and standardize across functions prior to merge. Tracked as "Robustness of stackSave / restore and error recovery the api should enable" in #466
| var newDimensionsLength = newDimensions.length; | ||
| var dimensionsPointer = Module.stackAlloc(newDimensionsLength * LENGTH_SIZE); | ||
| var i, currentDimension; | ||
| for (i = 0; i < newDimensionsLength; i += 1) { |
There was a problem hiding this comment.
Is a linting rule in .eslintrc.js. Why is preferred? Because ++ are subject to auto-semicolon insertion. Checkout eslint docs. If we really don't like it, there's also the exception "allowForLoopAfterthoughts": true
| @@ -279,20 +279,29 @@ VIREO_EXPORT Int32 EggShell_GetArrayDimLength(TypeManagerRef tm, const char* viN | |||
| } | |||
| //------------------------------------------------------------ | |||
| //! Resizes a variable size Array symbol to have new dimension lengths specified by newLengths, it also initializes cells for non-flat data. | |||
| //! Returns -1 if the symbols is not found, -2 if was not possible to resize the array and 0 if resizing was successful. | |||
| VIREO_EXPORT Int32 EggShell_ResizeArray(TypeManagerRef tm, const char* viName, const char* eltName, Int32 rank, Int32* newLengths) | |||
| VIREO_EXPORT EggShellResult EggShell_ResizeArray(TypeManagerRef tm, const TypeRef actualType, const void* pData, | |||
There was a problem hiding this comment.
None of the other functions in CEntryPoints.cpp use const for the TypeManagerRef. Maybe we can submit that as a separate change before merging ni:api-type-refactor to master.
Changes the existing resizeArray implementation to use the valueRef interface. Also allows getArrayDimensions to work with all valid Array types.